-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added capability of overriding wav MIDIPitchFraction with the Pipe999MIDIPitchFraction key https://github.com/GrandOrgue/grandorgue/issues/1378 #1396
Conversation
@oleg68 No, it's not working, the resulting pitch is wrong (one semitone too high). Remember: MIDIKeyNumber and PitchFraction should describe existing pitch - it's not used for adjusting pitch after the temperament but used to retune to exact standard pitch before temperament is applied. Also, I get loads of errors in the log about pitch is going to change more than 1800 cent after temperament - which doesn't happen... It's like the harmonic number isn't accounted for or something like that. |
Could you give an example?
Yes, I agree. But FOR TESTING PURPOSES ONLY you may to set MIDIKeyNumber to another value for checking that it works. |
@oleg68 I've created a sample set (just for you!) that demonstrates how the pitch from odf is broken both with the standard correction and with the fraction of this PR. Pay very close attention to how the left hand stops with embedded pitch info behaves when you change to another temperament than original! The goal is to make the right hand stops do exactly the same. The pitch info specified in odfs and the embedded pitch in the wav is exactly the same in each case and I expect that they behave exactly the same - nothing else. |
By the way. The reason we need separate PitchFraction and PitchCorrection is that while the fraction describes the pitch and thus puts the sample in exact standard equal before applying the temperament, the correction afterwards will apply an adjustment further away. This is necessary for undulating stops like Voix Celeste and Unda Maris etc (and also simply for correcting faulty pitch information). |
The new key is called |
Got it! Yes that works, but I still get errors that the temperament will change the pitch more than 1800 cent for every pipe with odf specified pitch info. Clearly that doesn't happen. |
@larspalo I fixed help and validation. Now your example works as expected: after switchin to any not-original temperament all six stops sound equally. I couldn't find any problems with MIDIKeyNumber. |
@oleg68 Great! Thanks a lot for fixing this! I'll test the PR later when the build is finished. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is excellent! The only thing I would consider is changing the help text slightly to be more precise of the intent of the different settings.
@oleg68 I think it's very good, except for that you included one of my comments in parenthesis that was just a minor suggestion that I thought the word "when" was better here than "for". If you don't like it - just keep the word for. |
I changed to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for fixing this! I appreciate the effort! This issue was perhaps not very big in the greater picture, but it was an annoyance that it wasn't working as it should. Now it does, and I'm happy with it!
@rousseldenis could you approve this PR? |
511e6e3
to
17b9862
Compare
17b9862
to
e0306be
Compare
Resolves: #1378
As discussed in #1378, Pipe999PitchCorrection does not override the wav PitchFraction. This PR introduces the new ODF key Pipe999MIDIPitchFraction that is used instead of the wav PitchFraction.